Add ip filter design doc #4990

ecordell · 2023-01-19T15:59:06Z

I took a stab at a proposal to address #3693

I haven't looked deeply at the envoy filters available for this, but I wrote down what looked like would work based on the docs.

github-actions · 2023-01-19T16:01:38Z

Hi @ecordell! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

codecov · 2023-01-19T19:39:35Z

Codecov Report

Merging #4990 (79205d3) into main (be9de2c) will not change coverage.
The diff coverage is n/a.

❗ Current head 79205d3 differs from pull request most recent head af885f2. Consider uploading reports for the commit af885f2 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4990   +/-   ##
=======================================
  Coverage   77.94%   77.94%           
=======================================
  Files         138      138           
  Lines       17731    17731           
=======================================
  Hits        13820    13820           
  Misses       3646     3646           
  Partials      265      265

sunjayBhatia

it would be great to add some more implementation notes about how the Envoy filters might work, once we agree on a general shape of the API, you've got a nice example in there but as we go we may want to add some more the that section

i.e. mention the RBAC filter: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rbac_filter and how configuring Prinicipals might work: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/rbac/v3/rbac.proto#envoy-v3-api-msg-config-rbac-v3-principal maybe add some links to the API reference in case readers want to take a look

some examples of configuration and requests (including source ip and/or x-forwarded-for header) that would be allowed/denied would be great to add

the Envoy filter allows full per-route configuration so the per-route configuration makes a lot of sense to me

sunjayBhatia · 2023-01-20T00:23:41Z

design/ip-filtering-design.md

+- Support filtering on request IP or on forwarded IP (i.e. via `X-Forwarded-For`)
+
+## Non Goals
+- Define Gateway API support for ip filtering. This is an obvious next step, but requires broader agreement.


we'll likely need to write up a design for a CR to be used as an extension Filter or Policy for Gateway API

Are you saying that you think we should bring the Gateway API into scope? Or just that that's the likely direction if/when ip filtering is added to Gateway API?

addressing a bit of both actually, probably good to get a baseline of what we want to support done in HTTPProxy only to start with (so no Gateway API work here) as well as acknowledging that unless the upstream Gateway API project supports it natively we will have to write our own CR to handle that feature

we can follow on later with designs on what to do w/ Gateway API

sunjayBhatia · 2023-01-20T00:24:40Z

design/ip-filtering-design.md

+Note that:
+- Allow rules cannot be mixed with Deny rules: it won't be evident if a deny rule should be an exception to an allow rule, or if an allow rule should be an exception to a deny rule.
+- Multiple allow/deny CIDR ranges can be specified by repeating a key.
+- `peer` refers to the request IP


writing down how exactly these concepts fold into the Envoy RBAC configuration fields would be good to make sure to head off any confusion early

Is the terminology peer and remote originating from Ambassador? Envoy seems to use direct_remote_ip and remote_ip. Which one is better to align?

Yep, I took that from Ambassador (it was linked in one of the original GH issues on ip filtering). I don't have a strong opinion on the fields names, if direct_remote and remote sound better.

Is the terminology peer and remote originating from Ambassador? Envoy seems to use direct_remote_ip and remote_ip. Which one is better to align?

I like the latter

ecordell · 2023-01-20T18:19:36Z

Thanks for the review @sunjayBhatia - I'll work on ironing out the envoy details and update here.

tsaarni

Great design doc @ecordell, Thank You for working on this! I've added some comments/questions.

tsaarni · 2023-01-21T09:21:42Z

design/ip-filtering-design.md

+
+## Goals
+
+- Support ip allowlists and denylists via Contour APIs.


It could be mentioned that IPv4 and IPv6 in both single IPs and CIDR blocks should be supported (comment from @xaleeks in here).

design/ip-filtering-design.md

tsaarni · 2023-01-21T09:47:13Z

design/ip-filtering-design.md

+```
+
+Note that:
+- Allow rules cannot be mixed with Deny rules: it won't be evident if a deny rule should be an exception to an allow rule, or if an allow rule should be an exception to a deny rule.


This means allow rule starts from "deny all" right? Maybe mention that explicitly?

tsaarni · 2023-01-21T09:48:13Z

design/ip-filtering-design.md

+Note that:
+- Allow rules cannot be mixed with Deny rules: it won't be evident if a deny rule should be an exception to an allow rule, or if an allow rule should be an exception to a deny rule.
+- Multiple allow/deny CIDR ranges can be specified by repeating a key.
+- `peer` refers to the request IP


Is the terminology peer and remote originating from Ambassador? Envoy seems to use direct_remote_ip and remote_ip. Which one is better to align?

ecordell · 2023-01-25T23:58:42Z

@sunjayBhatia @tsaarni I addressed most of the feedback and made a couple of updates to the proposed API.

I've implemented this in #5008 with the understanding that we may need to adjust it if we change anything here first. Implementing it was the easiest way to suss out exactly how envoy's rbac filter actually works in practice.

ecordell · 2023-01-30T14:11:03Z

Since I think the API design is what probably requires the most discussion, here are some alternatives to think about:

What's in the proposal right now:

      ipAllowPolicy:
      - cidr: 127.0.0.1/32
        source: Peer
      #or 
      ipDenyPolicy:
      - cidr: 99.99.0.0/16
        source: Remote

separate top-level fields for allow vs. deny
separate field for cidr range and ip source
uses Remote / Peer terminology instead of Remote and DirectRemote

An option with only one top-level field:

      ipFilterPolicy:
      - allow: 127.0.0.1/32
        source: Peer
      - allow: 99.99.0.0/16
        source: Peer
      #or 
      ipFilterPolicy:
      - deny: 99.99.0.0/16
        source: Remote

single top-level field for filters, each filter specifies allow vs. deny
separate field for cidr range and ip source
uses Remote / Peer terminology instead of Remote and DirectRemote

Using envoy terminology for remotes:

      ipFilterPolicy:
      - allow: 127.0.0.1/32
        source: DirectRemote
      - allow: 99.99.0.0/16
        source: Remote
      #or 
      ipFilterPolicy:
      - deny: 99.99.0.0/16
        source: Remote

single top-level field for filters, each filter specifies allow vs. deny
separate field for cidr range and ip source
uses Remote and DirectRemote

If the top-level field indicates allow vs. deny, we can get by with only one field for the actual filters:

      ipAllowPolicy:
      - directRemote: 127.0.0.1/32
      - remote: 99.99.0.0/16
      #or 
      ipDenyPolicy:
      - remote: 99.99.0.0/16

separate top-level fields for allow vs. deny
single field for ip source and range

Another api question is whether bare ips should be interpreted as /32 cidrs. Right now I have everything written as an explicit cidr range.

sunjayBhatia · 2023-02-01T23:14:17Z

Another api question is weather bare ips should be interpreted as /32 cidrs. Right now I have everything written as an explicit cidr range.

I'm in favor of being more explicit, I don't think it's too bad to ask people to add /32 or /128 if they want to allow/deny a particular IP

sunjayBhatia · 2023-02-01T23:23:14Z

Also I'm somewhat in favor of the options that have only one top-level field, just so there are less keys to have to configure and people don't get the impression just looking at the shape of the API that they could combine allow and deny policies on a route

ecordell · 2023-02-03T22:59:19Z

I'm in favor of being more explicit, I don't think it's too bad to ask people to add /32 or /128 if they want to allow/deny a particular IP

I tend to agree, but for what it's worth the nginx whitelist feature supports single-ips in the filter.

izturn · 2023-02-07T09:41:32Z

@ecordell any progress?

tsaarni · 2023-02-08T07:31:15Z

I'm in favor of being more explicit, I don't think it's too bad to ask people to add /32 or /128 if they want to allow/deny a particular IP

I tend to agree, but for what it's worth the nginx whitelist feature supports single-ips in the filter.

iptables -s address[/mask] also allows using single IP address, from man page: "Address can be either a network name, a hostname, a network IP address (with /mask), or a plain IP address.". It seems to be pretty common shorthand with some routers as well (like here).

ecordell · 2023-02-14T15:58:56Z

@tsaarni I agree, allowing bare IPs probably makes the most sense.

@izturn I believe I've addressed all of the feedback in this PR; I'm just waiting for further review and approval. Is there something specific you'd like to see addressed?

izturn · 2023-02-15T02:32:19Z

@ecordell, thx for your work, I can't wait to use it, btw I saw there are two branches: ipfilter, annotation-ipfilter in your fork, which one is your prefer?

ecordell · 2023-02-15T15:37:46Z

Thanks @izturn!

ipfilter is the branch we should look at - annotation-ipfilter is a fork that uses annotations to drive the ipfiltering so that we can use the ipfiltering features before this proposal is approved and merged.

izturn · 2023-02-16T02:41:27Z

got it

design/ip-filtering-design.md

sunjayBhatia

looks like adding support for per-virtualhost filtering is something people are enthusiastic about as well

we can move forward with the design as-is and add to it in a follow-up PR or add it here, up to you @ecordell

github-actions · 2023-03-17T00:21:20Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

ecordell · 2023-03-18T14:03:55Z

@sunjayBhatia I'll work on updating for virtualhost filtering, sorry for the silence

artificial-aidan · 2023-03-20T16:59:03Z

Late to the party on this one. Is supporting the ingress.kubernetes.io/whitelist-source-range annotation on ingress objects in the scope of this design? If we are adding IP filtering then it would be great to be a drop in replacement for other ingress replacements. That annotation is the only reason I keep an nginx controller running in our clusters.

sunjayBhatia · 2023-03-20T19:53:05Z

Late to the party on this one. Is supporting the ingress.kubernetes.io/whitelist-source-range annotation on ingress objects in the scope of this design? If we are adding IP filtering then it would be great to be a drop in replacement for other ingress replacements. That annotation is the only reason I keep an nginx controller running in our clusters.

not at the moment, no, but if you're willing to contribute an addendum to this document or an additional feature once this is merged/agreed upon we will of course consider it

ecordell · 2023-03-20T22:57:54Z

@sunjayBhatia doc is updated for virtualhost-level ip filtering

It's implemented in #5008 as well

wilsonwu

lgtm

izturn · 2023-03-22T07:47:13Z

design/ip-filtering-design.md

+## Goals
+
+- Support ipv4 and ipv6 allowlists and denylists via Contour APIs.
+- Support filtering on request IP or on forwarded IP (i.e. via `X-Forwarded-For`)


From the change log:

Filters can indicate whether the direct IP should be used or whether a reported IP from PROXY or X-Forwarded-For should be used instead.

maybe we need to unify them all

maybe we need to unify them all

Not sure I'm following your suggestion, would you mind rephrasing it?

This proposal is to use the envoy implementation, which does support PROXY in addition to X-Forwarded-For, if that's your question?

Sorry for my pool english, I means the descriptions in these two places need to be consistent.

izturn

/lgtm

sunjayBhatia

Taking this and the implementation in #5008 together I think this is good to approve, the open questions I had that weren't explicitly covered here were around how exactly the overrides would work (virtualhost config overridden per-route) and how includes would work, but looks like that is already tested in the implementation so we can discuss there

I'll leave this open for a little longer in case anyone else wants to chime in

design/ip-filtering-design.md

sunjayBhatia

one more thought I just had would be to nail down what the precedence of the RBAC filter is when combined with other filters

i.e. should IP filtering happen before rate limiting, ExtAuth etc.

since the filter ends up getting applied as a per-route filter this might mean it is evaluated later than people would expect

skriss

One small clarification for posterity

design/ip-filtering-design.md

Signed-off-by: Evan Cordell <[email protected]>

ecordell requested a review from a team as a code owner January 19, 2023 15:59

ecordell requested review from stevesloka and skriss and removed request for a team January 19, 2023 15:59

ecordell force-pushed the ipfilterdesign branch from beb0c4e to 5678b08 Compare January 19, 2023 16:22

skriss added kind/design Categorizes issue or PR as related to design. release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. labels Jan 19, 2023

skriss requested review from sunjayBhatia and tsaarni January 19, 2023 19:30

sunjayBhatia reviewed Jan 20, 2023

View reviewed changes

tsaarni reviewed Jan 21, 2023

View reviewed changes

ecordell force-pushed the ipfilterdesign branch from 5678b08 to 9142ba7 Compare January 25, 2023 23:54

ecordell mentioned this pull request Jan 25, 2023

httpproxy: Add support for ip-based filtering #5008

Merged

sunjayBhatia requested review from tsaarni and sunjayBhatia January 26, 2023 23:19

wilsonwu reviewed Feb 21, 2023

View reviewed changes

design/ip-filtering-design.md Show resolved Hide resolved

sunjayBhatia reviewed Mar 2, 2023

View reviewed changes

bison mentioned this pull request Mar 9, 2023

Add IP allowlist and blocklist functionality #3693

Closed

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 17, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 19, 2023

ecordell force-pushed the ipfilterdesign branch from 9142ba7 to 79205d3 Compare March 20, 2023 22:56

sunjayBhatia requested review from sunjayBhatia, wilsonwu and izturn and removed request for wilsonwu March 21, 2023 00:09

wilsonwu approved these changes Mar 21, 2023

View reviewed changes

izturn reviewed Mar 22, 2023

View reviewed changes

izturn approved these changes Mar 22, 2023

View reviewed changes

sunjayBhatia approved these changes Mar 24, 2023

View reviewed changes

design/ip-filtering-design.md Outdated Show resolved Hide resolved

sunjayBhatia reviewed Mar 24, 2023

View reviewed changes

skriss reviewed Mar 24, 2023

View reviewed changes

design/ip-filtering-design.md Outdated Show resolved Hide resolved

add ip filter design doc

af885f2

Signed-off-by: Evan Cordell <[email protected]>

ecordell force-pushed the ipfilterdesign branch from 79205d3 to af885f2 Compare March 27, 2023 14:59

skriss approved these changes Mar 27, 2023

View reviewed changes

skriss merged commit 4ed05fa into projectcontour:main Mar 28, 2023

ecordell mentioned this pull request May 11, 2023

Enhancement: add HTTPRoute IP ACL's kubernetes-sigs/gateway-api#1141

Closed


		## Goals

		- Support ip allowlists and denylists via Contour APIs.

Add ip filter design doc #4990

Add ip filter design doc #4990

Conversation

ecordell commented Jan 19, 2023

github-actions bot commented Jan 19, 2023

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

sunjayBhatia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

sunjayBhatia Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Jan 20, 2023

tsaarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Jan 25, 2023 • edited Loading

ecordell commented Jan 30, 2023 • edited Loading

sunjayBhatia commented Feb 1, 2023

sunjayBhatia commented Feb 1, 2023

ecordell commented Feb 3, 2023

izturn commented Feb 7, 2023

tsaarni commented Feb 8, 2023

ecordell commented Feb 14, 2023

izturn commented Feb 15, 2023

ecordell commented Feb 15, 2023

izturn commented Feb 16, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2023

ecordell commented Mar 18, 2023

artificial-aidan commented Mar 20, 2023

sunjayBhatia commented Mar 20, 2023

ecordell commented Mar 20, 2023

wilsonwu left a comment

Choose a reason for hiding this comment

izturn Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

ecordell Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

izturn Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

izturn left a comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2023 •

edited

Loading

sunjayBhatia left a comment •

edited

Loading

ecordell Jan 20, 2023 •

edited

Loading

sunjayBhatia Jan 20, 2023 •

edited

Loading

ecordell commented Jan 25, 2023 •

edited

Loading

ecordell commented Jan 30, 2023 •

edited

Loading

izturn Mar 22, 2023 •

edited

Loading

ecordell Mar 22, 2023 •

edited

Loading

izturn Mar 23, 2023 •

edited

Loading